Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add netstat sample application #40

Merged
merged 11 commits into from
Jan 4, 2024
Merged

Add netstat sample application #40

merged 11 commits into from
Jan 4, 2024

Conversation

dtrugman
Copy link
Owner

An alternative suggestion to #38 and #39.

@dtrugman dtrugman added the enhancement New feature or request label Dec 14, 2023
@dtrugman dtrugman self-assigned this Dec 14, 2023
@dtrugman dtrugman mentioned this pull request Dec 14, 2023
Closed
@rm5248
Copy link

rm5248 commented Jan 2, 2024

Note: this currently fails to build for me with g++ 12 / clang++ 14(using Debian 12). It's not immediately obvious to me why it fails on these types and it doesn't for the other types. Here's the clang error(g++ errors on the same lines):

[ 48%] Building CXX object CMakeFiles/sample.dir/sample/enum_block.cpp.o
In file included from /home/robert/pfs/sample/enum_block.cpp:18:
/home/robert/pfs/sample/log.hpp:71:5: error: call to function 'operator<<' that is neither visible in the template definition nor found by argument-dependent lookup
    LOG(value);
    ^
/home/robert/pfs/sample/log.hpp:25:28: note: expanded from macro 'LOG'
#define LOG(...) std::cout << __VA_ARGS__ << ENDL
                           ^
/home/robert/pfs/sample/enum_block.cpp:38:9: note: in instantiation of function template specialization '__print<pfs::block_stat>' requested here
        print(stat);
        ^
/home/robert/pfs/sample/log.hpp:84:18: note: expanded from macro 'print'
#define print(x) __print(#x, x)
                 ^
/home/robert/pfs/sample/format.hpp:731:22: note: 'operator<<' should be declared prior to the call site or in namespace 'pfs'
inline std::ostream& operator<<(std::ostream& out,
                     ^
In file included from /home/robert/pfs/sample/enum_block.cpp:18:
/home/robert/pfs/sample/log.hpp:71:5: error: call to function 'operator<<' that is neither visible in the template definition nor found by argument-dependent lookup
    LOG(value);
    ^
/home/robert/pfs/sample/log.hpp:25:28: note: expanded from macro 'LOG'
#define LOG(...) std::cout << __VA_ARGS__ << ENDL
                           ^
/home/robert/pfs/sample/enum_block.cpp:41:9: note: in instantiation of function template specialization '__print<pfs::block_queue>' requested here
        print(queue);
        ^
/home/robert/pfs/sample/log.hpp:84:18: note: expanded from macro 'print'
#define print(x) __print(#x, x)
                 ^
/home/robert/pfs/sample/format.hpp:753:22: note: 'operator<<' should be declared prior to the call site or in namespace 'pfs'
inline std::ostream& operator<<(std::ostream& out,
                     ^
2 errors generated.

One possibly confusing thing: it seems that the filter returns true if the socket should be discarded. To me, true should be that we want to keep this, and false means we don't. Perhaps turn this into an enum?

enum class FilterResponse{
  Accept,
  Reject
};

@dtrugman
Copy link
Owner Author

dtrugman commented Jan 2, 2024

Thanks for the useful comments @rm5248.
The compilation was indeed broken with newer compilers. Reordering two includes fixed that.
I also agree with your comment. I was sure that I used "true" as filter in, but it turns out I didn't. Used an enum to make it explicit.

rm5248
rm5248 reviewed Jan 3, 2024
View reviewed changes
sample/tool_netstat.cpp Outdated
Comment on lines 34 to 36
return inodes.find(sock.inode) == inodes.end()
? pfs::filter::action::keep
: pfs::filter::action::drop;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value here is backwards, it should be:

Suggested change
return inodes.find(sock.inode) == inodes.end()
? pfs::filter::action::keep
: pfs::filter::action::drop;
return inodes.find(sock.inode) == inodes.end()
? pfs::filter::action::drop
: pfs::filter::action::keep;

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are totally right. Thanks for noticing. Fixed.

@rm5248
Copy link

rm5248 commented Jan 3, 2024

This looks pretty good to me; I think the ability to add in the custom filters is a good trade-off to make it easy to filter things from only one process while still making it be "just parse /proc" and simple in that regard.

@dtrugman
Copy link
Owner Author

dtrugman commented Jan 4, 2024

Thank you! I'm happy you find this resolution suitable!
Will push the code, bump the version, and release it.

@dtrugman dtrugman merged commit 4e1276e into master Jan 4, 2024
1 check passed
@dtrugman dtrugman deleted the feat/sample-netstat branch January 4, 2024 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rm5248 rm5248 rm5248 left review comments

Assignees

@dtrugman dtrugman

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dtrugman @rm5248